Skip to content

[SPARK-25581][SQL] Rename method benchmark as runBenchmarkSuite in BenchmarkBase #22599

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
gengliangwang wants to merge 3 commits into from
Closed

[SPARK-25581][SQL] Rename method benchmark as runBenchmarkSuite in BenchmarkBase #22599

gengliangwang wants to merge 3 commits into from

Conversation

@gengliangwang
Copy link
Member

@gengliangwang gengliangwang commented Oct 1, 2018 •
edited
Loading

What changes were proposed in this pull request?

Rename method benchmark in BenchmarkBase as runBenchmarkSuite . Also add comments.
Currently the method name benchmark is a bit confusing. Also the name is the same as instances of Benchmark:

spark/sql/hive/src/test/scala/org/apache/spark/sql/hive/orc/OrcReadBenchmark.scala

Lines 330 to 339 in f246813

benchmark.addCase("Hive built-in ORC") { _ =>
spark.sql(s"SELECT sum(c$middle) FROM hiveOrcTable").collect()
}
benchmark.run()
}
}
}
override def benchmark(): Unit = {

How was this patch tested?

Unit test.

@gengliangwang
Copy link
Member Author

gengliangwang commented Oct 1, 2018

@wangyum @dongjoon-hyun @cloud-fan

dongjoon-hyun
dongjoon-hyun reviewed Oct 1, 2018
View reviewed changes
core/src/test/scala/org/apache/spark/benchmark/BenchmarkBase.scala Outdated
Copy link
Member

@dongjoon-hyun dongjoon-hyun Oct 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got the idea of this PR. But, for me, benchmarkSuite looks like a test suite again. Isn't it confusing in another way.

Copy link
Member Author

@gengliangwang gengliangwang Oct 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it means a collection of benchmarks. Just like TestSuite is a collection of test cases.

Copy link
Member Author

@gengliangwang gengliangwang Oct 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the word suite, it doesn't have to be about testing.

jiangxb1987
jiangxb1987 approved these changes Oct 1, 2018
View reviewed changes
Copy link
Contributor

@jiangxb1987 jiangxb1987 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@SparkQA
Copy link

SparkQA commented Oct 1, 2018

Test build #96816 has finished for PR 22599 at commit 289ee3e.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Oct 1, 2018

Test build #96818 has finished for PR 22599 at commit 78e48ea.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@gengliangwang gengliangwang changed the title [SPARK-25581][SQL] Rename method benchmark as benchmarkSuite in BenchmarkBase [SPARK-25581][SQL] Rename method benchmark as runBenchmarkSuite in BenchmarkBase Oct 2, 2018
@gengliangwang
Copy link
Member Author

gengliangwang commented Oct 2, 2018

Discuss with @cloud-fan offline. Rename method benchmark as runBenchmarkSuite. Also add comment to guide developers to use runBenchmark for each scenario in implementations.

@dongjoon-hyun @wangyum Is this OK to you?

@wangyum
Copy link
Member

wangyum commented Oct 2, 2018

Thanks @gengliangwang I agree with you. It’s a good change.

@SparkQA
Copy link

SparkQA commented Oct 2, 2018

Test build #96855 has finished for PR 22599 at commit 1c15c25.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

dongjoon-hyun
dongjoon-hyun approved these changes Oct 2, 2018
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM. Merged to master.

@asfgit asfgit closed this in 7b4e94f Oct 2, 2018
@dongjoon-hyun dongjoon-hyun mentioned this pull request Oct 2, 2018
Closed
jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
@gengliangwang
[SPARK-25581][SQL] Rename method benchmark as runBenchmarkSuite i…
22f3194 
…n `BenchmarkBase`

## What changes were proposed in this pull request?

Rename method `benchmark` in `BenchmarkBase` as `runBenchmarkSuite `. Also add comments.
Currently the method name `benchmark` is a bit confusing. Also the name is the same as instances of `Benchmark`:

https://github.com/apache/spark/blob/f246813afba16fee4d703f09e6302011b11806f3/sql/hive/src/test/scala/org/apache/spark/sql/hive/orc/OrcReadBenchmark.scala#L330-L339

## How was this patch tested?

Unit test.

Closes apache#22599 from gengliangwang/renameBenchmarkSuite.

Authored-by: Gengliang Wang <gengliang.wang@databricks.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jiangxb1987 jiangxb1987 jiangxb1987 approved these changes

@wangyum wangyum wangyum approved these changes

@dongjoon-hyun dongjoon-hyun dongjoon-hyun approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@gengliangwang @SparkQA @wangyum @jiangxb1987 @dongjoon-hyun